Add investment trade and holding support#3
Conversation
Sure's backend already exposes /api/v1/trades and /api/v1/holdings for managing stock buys/sells/dividends/deposits/withdrawals/interest on investment (or crypto exchange) accounts, but this MCP server only wrapped the generic cash /api/v1/transactions endpoint. Investment accounts had no way to record trades through Claude/an MCP client. Adds 7 new tools mirroring the existing transaction tools' pattern: - get_trades / get_trade: list and view trades - create_trade: record a buy/sell/dividend/deposit/withdrawal/interest - update_trade / delete_trade: modify or remove a trade - get_holdings / get_holding: view computed stock positions (read-only, Sure derives these from trades + prices, so there's no write endpoint) Trade type validation (buy/sell/dividend/deposit/withdrawal/interest) and per-type required-field checks are left to the Sure API itself rather than duplicated client-side, consistent with how the existing transaction tools already defer validation to the server. README updated with the new tools table entries and a short "Investment Trades" section documenting which fields are required per trade_type, matching Api::V1::TradesController's actual validation.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe MCP server adds trade listing, retrieval, creation, updating, and deletion tools, plus read-only holding retrieval tools. The README documents these tools, trade type requirements, and holding limitations. ChangesInvestment data tools
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPTool
participant HTTPClient
participant SureAPI
MCPTool->>HTTPClient: get_client()
MCPTool->>SureAPI: Request trades or holdings endpoint
SureAPI-->>MCPTool: API response
MCPTool->>MCPTool: handle_response and serialize JSON
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sure_mcp_server/server.py`:
- Around line 389-394: Update the trade normalization around the response
handling at src/sure_mcp_server/server.py#L389-L394 and the holdings
normalization at src/sure_mcp_server/server.py#L632-L637 to branch on
isinstance(data, dict) before calling .get(); preserve direct list responses
as-is, while extracting the nested trades or holdings only from dictionaries.
- Line 577: Encode caller-controlled trade_id and holding_id values as single
URL path segments, rejecting or normalizing "." and ".." before encoding. Apply
this to src/sure_mcp_server/server.py lines 410, 554-557, 577, and 653, updating
each affected API request construction while preserving the existing endpoints
and request behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af990990-e6a0-4ceb-b6f8-a40dbf214553
📒 Files selected for processing (2)
README.mdsrc/sure_mcp_server/server.py
- get_trades/get_holdings: guard the paginated-response unwrap with isinstance(data, dict) before calling .get() on it, so a (currently hypothetical, but API-shape-wise possible) direct list response is passed through as-is instead of raising AttributeError. - get_trade/update_trade/delete_trade/get_holding: percent-encode trade_id/holding_id via a new encode_path_id() helper before interpolating them into the request path. httpx resolves relative paths against base_url like a browser resolves links, so an unescaped "/" would add path segments and a bare "."/".." would be collapsed as a dot-segment -- either way the request could land on a different endpoint than intended. "." and ".." are rejected outright since percent-encoding alone doesn't change them (they're unreserved characters). Both fixes verified: encode_path_id() unit-checked for a normal UUID (passthrough), a slash (encoded to %2F), and "."/".."/"" (rejected), plus a live round-trip of get_trades -> get_trade and get_holdings -> get_holding against a real Sure instance to confirm real IDs still resolve correctly end to end.
|
Addressed both actionable CodeRabbit findings in 3cd3fee:
Verified with unit checks on |
1 participant
Why
Sure's API already has full support for investment trades (
/api/v1/trades) and holdings (/api/v1/holdings) — buying/selling stock, dividends, deposits/withdrawals, interest — but this MCP server only ever wrapped the generic cash/api/v1/transactionsendpoint. There was no way to record a stock trade on an investment account through Claude/an MCP client; the only option (create_transaction) has no concept of ticker/quantity/price, so it can't represent a trade correctly.What's added
7 new tools, following the exact same pattern already used by the transaction tools (
get_client()/handle_response()/ try-except-log-json.dumps):get_tradesGET /api/v1/trades(filters:account_id,account_ids,start_date,end_date, paginated)get_tradeGET /api/v1/trades/:idcreate_tradePOST /api/v1/trades—trade_typeone ofbuy/sell/dividend/deposit/withdrawal/interestupdate_tradePATCH /api/v1/trades/:iddelete_tradeDELETE /api/v1/trades/:idget_holdingsGET /api/v1/holdings(read-only — Sure computes these from trades + prices)get_holdingGET /api/v1/holdings/:idAll endpoint paths, required params, and per-
typevalidation rules were confirmed directly againstApi::V1::TradesController/Api::V1::HoldingsControllerinwe-promise/sure, not guessed.Trade-type validation and per-type required-field rules (e.g.
qty/pricefor buy/sell,amountfor dividend/interest/deposit/withdrawal) are intentionally not duplicated client-side — same as the existing transaction tools, invalid input is left to the Sure API to reject with its own descriptive error message, so this stays in sync automatically if Sure's rules change.Docs
README.md: added the new tools to the tools table, plus a short "Investment Trades" section documenting which fields are required pertrade_typeand the investment/crypto-exchange account requirement.Testing
get_trades/get_holdings(and their singular counterparts) were run live against a real self-hosted Sure instance with existing investment-account data and correctly returned real trades/holdings.create_transaction/update_transaction/delete_transactiontools, with parameters verified against the controller source (linked above).python -m py_compileandruff check --select F,E9pass on the new code (pre-existing lint/type debt elsewhere in the file — untyped@mcp.tool()decorators, missingmcp/httpxstubs, a few long log lines — is unrelated to this change and left as-is to keep the diff focused).Note: this branch is also open as a PR against the upstream root repo, robcerda/sure-mcp-server (robcerda#4), since this fork's history forked from there. Happy to close that one if this is the canonical repo going forward.
Summary by CodeRabbit